feat: Set log level for Fetch/XHR breadcrumbs based on status code #13711

Zen-cronic · 2024-09-18T01:17:21Z

If you've added code that should be tested, please add tests.
Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Zen-cronic · 2024-09-18T01:29:57Z

packages/core/src/utils/breadcrumbsUtils.ts

+ * @param breadcrumb
+ */
+export function assignBreadcrumbLogLevel(breadcrumb: Breadcrumb): Breadcrumb {
+  const statusCode = breadcrumb.data?.status_code;


FetchBreadcrumbData and XhrBreadcrumbData are sent as the data property, which optionally contains status_code.

Lms24

Hey, thanks for (once again) contributing! :)

On first glance, this looks great to me! Would be amazing if you could add some tests for this. I think we should be able to test this in our Node integration-tests fairly well. (as well as maybe a couple of unit tests for the assignBreadcrumbLogLevel if you feel like doing extra credit work 😅)

I'm also gonna tag @lforst for a review since he has more context on this issue.

lforst

I would set the level in the place where we actually create the breadcrumbs, not in a central place downstream.

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

…Breadcrumb` is called Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Zen-cronic · 2024-09-18T15:44:51Z

packages/core/src/utils/breadcrumbsUtils.ts

+  } else if (statusCode >= 500) {
+    return 'error';
+  } else {
+    return 'info';


For the undefined and other cases, the fallback level is info. So, all the test cases for breadcrumbs in browser-integration-tests must be updated, which is quite a lot.

Is this behaviour needed? E.g., non-error request breadcrumb will have an additional property level: 'info'

Currently many tests are failing b/c of the default property. Maybe we shouldn't set it if the response has a non-error status code. Otherwise, it's a handful to fix those tests (though it's simply just adding the property).

Any thoughts?

For now, i've left out the default value (info) to not break the many other tests. But other sdks use a default value: getsentry/sentry-php@d12482a. So we should consider it.

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Modify `getBreadcrumbLogLevel` to account only for 4xx or 5xx status. Tests updated to reflect the changes to `getBreadcrumbLogLevel` function. Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

lforst

Thank you! :)

This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #13711 Co-authored-by: lforst <8118419+lforst@users.noreply.github.com>

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@sentry/node](https://github.com/getsentry/sentry-javascript/tree/master/packages/node) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.31.0` -> `8.32.0`](https://renovatebot.com/diffs/npm/@sentry%2fnode/8.31.0/8.32.0) | | [@sentry/react](https://github.com/getsentry/sentry-javascript/tree/master/packages/react) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.31.0` -> `8.32.0`](https://renovatebot.com/diffs/npm/@sentry%2freact/8.31.0/8.32.0) | --- ### Release Notes <details> <summary>getsentry/sentry-javascript (@sentry/node)</summary> ### [`v8.32.0`](https://github.com/getsentry/sentry-javascript/releases/tag/8.32.0) [Compare Source](getsentry/sentry-javascript@8.31.0...8.32.0) ##### Important Changes - **ref(browser): Move navigation span descriptions into op ([#13527](getsentry/sentry-javascript#13527 Moves the description of navigation related browser spans into the op, e.g. browser - cache -> browser.cache and sets the description to the performanceEntry objects' names (in this context it is the URL of the page). - **feat(node): Add amqplibIntegration ([#13714](getsentry/sentry-javascript#13714 - **feat(nestjs): Add `SentryGlobalGenericFilter` and allow specifying application ref in global filter ([#13673](getsentry/sentry-javascript#13673 Adds a `SentryGlobalGenericFilter` that filters both graphql and http exceptions depending on the context. - **feat: Set log level for Fetch/XHR breadcrumbs based on status code ([#13711](getsentry/sentry-javascript#13711 Sets log levels in breadcrumbs for 5xx to error and 4xx to warning. ##### Other Changes - chore(nextjs): Bump rollup to 3.29.5 ([#13761](getsentry/sentry-javascript#13761)) - fix(core): Remove `sampled` flag from dynamic sampling context in Tracing without Performance mode ([#13753](getsentry/sentry-javascript#13753)) - fix(node): Ensure node-fetch does not emit spans without tracing ([#13765](getsentry/sentry-javascript#13765)) - fix(nuxt): Use Nuxt error hooks instead of errorHandler to prevent 500 ([#13748](getsentry/sentry-javascript#13748)) - fix(test): Unflake LCP test ([#13741](getsentry/sentry-javascript#13741)) Work in this release was contributed by [@Zen-cronic](https://github.com/Zen-cronic) and [@Sjoertjuh](https://github.com/Sjoertjuh). Thank you for your contributions! ##### Bundle size 📦 | Path | Size | | ---------------------------------------------------------------- | ----------------- | | [@sentry/browser](https://github.com/sentry/browser) | 22.63 KB | | [@sentry/browser](https://github.com/sentry/browser) - with treeshaking flags | 21.42 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Tracing) | 34.86 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay) | 71.36 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay) - with treeshaking flags | 61.79 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay with Canvas) | 75.71 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay, Feedback) | 88.48 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay, Feedback, metrics) | 90.36 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. metrics) | 26.91 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Feedback) | 39.77 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. sendFeedback) | 27.29 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. FeedbackAsync) | 32.08 KB | | [@sentry/react](https://github.com/sentry/react) | 25.38 KB | | [@sentry/react](https://github.com/sentry/react) (incl. Tracing) | 37.84 KB | | [@sentry/vue](https://github.com/sentry/vue) | 26.8 KB | | [@sentry/vue](https://github.com/sentry/vue) (incl. Tracing) | 36.75 KB | | [@sentry/svelte](https://github.com/sentry/svelte) | 22.76 KB | | CDN Bundle | 23.94 KB | | CDN Bundle (incl. Tracing) | 36.63 KB | | CDN Bundle (incl. Tracing, Replay) | 71.13 KB | | CDN Bundle (incl. Tracing, Replay, Feedback) | 76.44 KB | | CDN Bundle - uncompressed | 70.14 KB | | CDN Bundle (incl. Tracing) - uncompressed | 108.6 KB | | CDN Bundle (incl. Tracing, Replay) - uncompressed | 220.48 KB | | CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed | 233.69 KB | | [@sentry/nextjs](https://github.com/sentry/nextjs) (client) | 37.8 KB | | [@sentry/sveltekit](https://github.com/sentry/sveltekit) (client) | 35.43 KB | | [@sentry/node](https://github.com/sentry/node) | 124.92 KB | | [@sentry/node](https://github.com/sentry/node) - without tracing | 93.57 KB | | [@sentry/aws-serverless](https://github.com/sentry/aws-serverless) | 103.28 KB | </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/161 Reviewed-by: Alexandre Soro <code@soro.dev> Co-authored-by: renovate <renovate@git.tristess.app> Co-committed-by: renovate <renovate@git.tristess.app>

Zen-cronic added 3 commits September 17, 2024 21:16

fix(core): Set log level for Fetch/XHR breadcrumbs based on status code

e70d1b3

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

chore(typo): Fix typo in Scope.addBreadcrumb

c141bd8

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Remove extra optional chain

fbcc140

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Zen-cronic commented Sep 18, 2024

View reviewed changes

Lms24 reviewed Sep 18, 2024

View reviewed changes

Lms24 requested a review from lforst September 18, 2024 07:12

lforst requested changes Sep 18, 2024

View reviewed changes

Zen-cronic added 3 commits September 18, 2024 10:17

test(core): Add unit test for the updated log level function

80efec1

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

fix(core): Apply breadcrumb log level function in packages where `add…

3100ead

…Breadcrumb` is called Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

fix(core): Revert to original addBreadcrumb code

7cea142

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

Zen-cronic commented Sep 18, 2024

View reviewed changes

Zen-cronic added 2 commits September 18, 2024 12:37

test(browser): Add sample integration test for breadcrumb log level

a71e07e

Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

fix(core): Add log level only to 4xx/5xx codes

bfa9caf

Modify `getBreadcrumbLogLevel` to account only for 4xx or 5xx status. Tests updated to reflect the changes to `getBreadcrumbLogLevel` function. Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>

lforst self-assigned this Sep 23, 2024

Clean up

e5e33ee

lforst changed the title ~~fix(core): Set log level for Fetch/XHR breadcrumbs based on status code~~ feat: Set log level for Fetch/XHR breadcrumbs based on status code Sep 23, 2024

lforst approved these changes Sep 23, 2024

View reviewed changes

lforst merged commit 1f898b6 into getsentry:develop Sep 23, 2024
125 checks passed

github-actions bot mentioned this pull request Sep 23, 2024

ref: Add external contributor to CHANGELOG.md #13749

Merged

github-actions bot mentioned this pull request Sep 26, 2024

Give Fetch/XHR breadcrumbs a log level based on status code #13359

Closed

FengShangWuQi mentioned this pull request Oct 23, 2024

[Snyk] Upgrade @sentry/gatsby from 7.20.0 to 8.32.0 FengShangWuQi/fengshangwuqi.github.io#1926

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Set log level for Fetch/XHR breadcrumbs based on status code #13711

feat: Set log level for Fetch/XHR breadcrumbs based on status code #13711

Zen-cronic commented Sep 18, 2024 •

edited

Loading

Zen-cronic Sep 18, 2024

Lms24 left a comment

lforst left a comment

Zen-cronic Sep 18, 2024 •

edited

Loading

Zen-cronic Sep 18, 2024

Zen-cronic Sep 22, 2024

lforst left a comment •

edited

Loading

feat: Set log level for Fetch/XHR breadcrumbs based on status code #13711

feat: Set log level for Fetch/XHR breadcrumbs based on status code #13711

Conversation

Zen-cronic commented Sep 18, 2024 • edited Loading

Zen-cronic Sep 18, 2024

Choose a reason for hiding this comment

Lms24 left a comment

Choose a reason for hiding this comment

lforst left a comment

Choose a reason for hiding this comment

Zen-cronic Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Zen-cronic Sep 18, 2024

Choose a reason for hiding this comment

Zen-cronic Sep 22, 2024

Choose a reason for hiding this comment

lforst left a comment • edited Loading

Choose a reason for hiding this comment

Zen-cronic commented Sep 18, 2024 •

edited

Loading

Zen-cronic Sep 18, 2024 •

edited

Loading

lforst left a comment •

edited

Loading